Merge generated-src and generated #117899
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
Team:Analytics
|
|
||
| tasks.register("regen") { | ||
| dependsOn "regenParser" | ||
| dependsOn "regenParser", "stringTemplates" |
There was a problem hiding this comment.
I had to ensure stringTemplates runs before other code generation.
This has to be done as stringTemplates task cleans the entire generated directory before execution (see StringTemplateTask) while other generation not.
This means that before this change /generated dir was not cleaned before generating new files, keeping files with corresponding templates deleted.
There was a problem hiding this comment.
Are you sure this is needed? The regen task uses a different mechanism and doesn't put the output in the same dir. It makes Antlr stuff everything into x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser.
I just removed the stringTemplates dependency locally and made a small change to our grammar - the diff looked very sane and contained; whereas with the dependency on stringTemplates in place, making changes to the grammar and running regen deletes a bunch of unrelated files, which one must not commit. I think that'll make making changes to the grammar a bit painful.
There was a problem hiding this comment.
I tried to group it with other gen stuff I noticed, but may be it is worth to update the stringTemplates not to clear files unconditionally and instead have dedicated clear generated files tasks. What gradle task do we use to generate other files in main/gnerated?
Antlr generated code goes into main/java, I do not think this is intended as well. I will check If I can move it to generated in a separate pr.
|
|
||
| tasks.register("regen") { | ||
| dependsOn "regenParser" | ||
| dependsOn "regenParser", "stringTemplates" |
There was a problem hiding this comment.
Are you sure this is needed? The regen task uses a different mechanism and doesn't put the output in the same dir. It makes Antlr stuff everything into x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser.
I just removed the stringTemplates dependency locally and made a small change to our grammar - the diff looked very sane and contained; whereas with the dependency on stringTemplates in place, making changes to the grammar and running regen deletes a bunch of unrelated files, which one must not commit. I think that'll make making changes to the grammar a bit painful.
| } | ||
| } | ||
|
|
||
| tasks.named("spotlessJava") { dependsOn stringTemplates } |
There was a problem hiding this comment.
Why do we remove this dependency?
There was a problem hiding this comment.
As I was explaining above stringTemplates always cleans the generation target directory before execution. If it is executed after other generation tasks it will remove their results now as they are in the same directory.
I had to remove it from here as spotlessJava is executed rather late. Instead I moved stringTemplates as early as possible.
There was a problem hiding this comment.
We cannot have multiple tasks share an output directory. If we want to have a "single" generated source directory for the purposes of the IDE then we'll need to have another task aggregate them. This should actually be a Gradle deprecation warning if you attempt to do this. I'm surprised the build didn't complain.
The issue with having to strictly order tasks because they clean their output folder is one reason why this is generally bad practice. It's very easy to have tasks clobber eachothers output.
There was a problem hiding this comment.
I don't see any other task writing into src/main/generated in :x-pack:plugin:esql
I don't understand your explanation here why this change is needed. To me it looks like spotless is configured to ignore everything that is generated so we should be just safe to remove this task dependency as you did
There was a problem hiding this comment.
I also noticed: running our tests (which depends on regenerating the code) now deletes a buuunch of files. The tests still pass, so I assume that these were obsolete in the first place:
I think we should delete them in this PR so they don't pollute upcoming PRs.
These are the same files that get deleted when we run regen with this PR.
Update: Nah, we actually depend on these files.
|
@idegtiarenko , with this change we cannot jump to the generated evaluators from other parts of the code anymore. The tests still run fine (e.g. |
| } | ||
| } | ||
|
|
||
| tasks.named("spotlessJava") { dependsOn stringTemplates } |
There was a problem hiding this comment.
We cannot have multiple tasks share an output directory. If we want to have a "single" generated source directory for the purposes of the IDE then we'll need to have another task aggregate them. This should actually be a Gradle deprecation warning if you attempt to do this. I'm surprised the build didn't complain.
The issue with having to strictly order tasks because they clean their output folder is one reason why this is generally bad practice. It's very easy to have tasks clobber eachothers output.
|
Outdated |
Currently there are two (
generatedandgenerated-src) directories used for generated code.There is no good reason for it. This change merges them into one
generateddir.